[Task]32767979: Support AI Foundry by Handling GEN_AI_SYSTEM Attributes #41705
Conversation
Karlie-777
requested review from
lzchen,
jeremydvoss,
hectorhdzg,
rads-1996,
MSNev and
JacksonWeber
as code owners
...e-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_utils.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for handling the GEN_AI_SYSTEM attribute by prefixing span types with GenAI | ... and using the attribute as a fallback for HTTP dependency targets.
- Introduces fallback logic in
_get_target_and_path_for_http_dependencyto set the dependency target fromgen_ai.system. - Updates
_convert_span_to_envelopeto prefix span types withGenAI | <system>for both CLIENT and InProc spans. - Extends tests in
test_trace.pyto cover internal spans carrying thegen_ai.systemattribute.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sdk/monitor/azure-monitor-opentelemetry-exporter/tests/trace/test_trace.py | Adjusted client GenAI type assertion and added a new test for internal GenAI spans |
| sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_utils.py | Added a fallback to use gen_ai.system as the HTTP dependency target |
| sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_exporter.py | Prefixed CLIENT and InProc span types with `GenAI |
Comments suppressed due to low confidence (2)
sdk/monitor/azure-monitor-opentelemetry-exporter/tests/trace/test_trace.py:768
- [nitpick] The test name is quite long and blends 'client' and 'internal'. Consider renaming to something like
test_internal_span_with_gen_ai_system_typeto improve clarity.
def test_span_to_envelope_client_internal_genai_type(self):
sdk/monitor/azure-monitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_utils.py:207
- The comment indentation under the
elif gen_ai_attributes.GEN_AI_SYSTEMblock is misaligned. Please adjust the indentation to match the surrounding code block for consistency.
# If no fields are available to set target using standard rules, set Dependency Target to gen_ai.system if present
...onitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_exporter.py
Outdated
Show resolved
Hide resolved
...onitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_exporter.py
Outdated
Show resolved
Hide resolved
| @@ -407,7 +407,7 @@ def _convert_span_to_envelope(span: ReadableSpan) -> TelemetryItem: | |||
| span.attributes, | |||
| ) | |||
| elif gen_ai_attributes.GEN_AI_SYSTEM in span.attributes: # GenAI | |||
There was a problem hiding this comment.
These mappings take precedence over other mappings (ex. HTTP) even if their attributes are also present on the span.
This check needs to be moved up before the HTTP check.
There was a problem hiding this comment.
just curious, why it was added after other mappings originally?
There was a problem hiding this comment.
read the spec again, I assume it should mean gen_ai attribute value should override all other values (please correct me if I am wrong)
if we put gen_ai logic before http mapping, for example:
if attributes[gen_ai]
... // gen_ai mapping
if HTTP_REQUEST_METHOD in attributes // http mappings
... // other mappings
then the gen_ai value will be reset by other attribute values.
There was a problem hiding this comment.
so put gen_ai mapping after all other mappings, then gen_ai value will take precedence over other values
...onitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_exporter.py
Outdated
Show resolved
Hide resolved
|
Please add an entry to the CHANGELOG and link the spec. |
|
confirmed with @lzchen, no need to handle Dependency Standard Metrics, it will be auto handled by breeze endpoint |
...onitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_exporter.py
Outdated
Show resolved
Hide resolved
| @@ -760,8 +778,57 @@ def test_span_to_envelope_client_gen_ai(self): | |||
| self.assertEqual(envelope.data.base_data.result_code, "0") | |||
|
|
|||
| self.assertEqual(envelope.data.base_type, "RemoteDependencyData") | |||
| self.assertEqual(envelope.data.base_data.type, "az.ai.inference") | |||
| self.assertEqual(envelope.data.base_data.type, "N/A") | |||
There was a problem hiding this comment.
@lzchen once gen_ai mappings is set at the top, this test case will be returning "N/A" instead of gen_ai. Just to confirm, it is what we are expecting.
| if gen_ai_attributes.GEN_AI_SYSTEM in span.attributes: # GenAI | ||
| gen_ai_attributes_val = span.attributes[gen_ai_attributes.GEN_AI_SYSTEM] | ||
| if gen_ai_attributes_val: | ||
| data.type = _GEN_AI_ATTRIBUTE_PREFIX.format(gen_ai_attributes_val) | ||
| if _AZURE_SDK_NAMESPACE_NAME in span.attributes: # Azure specific resources |
...onitor-opentelemetry-exporter/azure/monitor/opentelemetry/exporter/export/trace/_exporter.py
Show resolved
Hide resolved
From Spec: https://github.com/aep-health-and-standards/Telemetry-Collection-Spec/blob/main/ApplicationInsights/genai_semconv_mapping.md